Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

chore(components): Converted 24 component files from class-based to functional #342

Merged
merged 6 commits into from
Oct 31, 2023
Merged

chore(components): Converted 24 component files from class-based to functional #342

merged 6 commits into from
Oct 31, 2023

Conversation

hwinsby
Copy link
Contributor

@hwinsby hwinsby commented Oct 12, 2023

Description

  • Updates component files from class-based to functional to allow use of React hooks.
  • Does not make any changes to the components' functionality, but provides cleaner and more readable syntax.
  • Altered certain variable names to avoid duplication of prop names in the same file. These name duplications worked with the class-based system, but clashed with functional-component syntax.

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR fix an open issue?

  • Yes
  • No

@hwinsby hwinsby force-pushed the hiw-functionalize-components branch from 56ae73f to 32f98d2 Compare October 12, 2023 00:33
bucko13
bucko13 suggested changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 This is awesome! Thanks so much for taking the time to do these upgrades. I have a couple small nits and comments. There's also a merge conflict to resolve. I'd like to try and do a full walk-thru as well but overall I didn't see any real red flags.

src/components/CreateAddress/HardwareWalletPublicKeyImporter.jsx
render = () => {
const { status, bip32Path, publicKeyError } = this.state;
const interaction = this.interaction();
const [status, setStatus] = useState(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this up to the other useStates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an oversight on my part. Line 41 uses this.interaction to initialize the state. I think the this. needs to be removed, but the state still has to be initialized after line 32 where we declare interaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, didn't even notice the this.

I think hoisting should make it irrelevant in terms of order. Something else to consider is that you can use useState or useCallback to set the interaction and set it to a default as well. That's probably safer anyway to handle the state changes as well anyway which addresses the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bucko13, I tried moving the state declaration to the top, but eslint is unhappy and throwing the eslint(no-use-before-define) rule. Would you want this rule turned off?

I updated interaction to be a useCallback in commit 4c7681f. I think this is what you were asking for, correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. No that's a fine rule to have.

And yeah, that's the change I was thinking of. In addition (or maybe instead?) as a way to keep the order consistent you can use the state to set the interaction, which I did in another component here for a similar conversion a while ago (obviously we could use more).

In any case, this is a non-blocking nit from me!

src/components/CreateAddress/index.jsx Outdated Show resolved Hide resolved
src/components/EditableName.jsx Outdated Show resolved Hide resolved
src/components/EditableName.jsx Outdated Show resolved Hide resolved
src/components/EditableName.jsx Outdated Show resolved Hide resolved
src/components/Navbar.jsx Outdated Show resolved Hide resolved
bucko13
bucko13 previously approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this looks good great to me! I still want to try and run through some user flows to confirm, and I also need to figure out how to coordinate with some other big PRs we have in the pipeline and hopefully coordinate one big release.

@bucko13
Copy link
Contributor

bucko13 commented Oct 30, 2023

Got a linting error to resolve:

/home/runner/work/caravan/caravan/src/components/CreateAddress/PublicKeyImporter.jsx
Error:   32:3  error  'resetPublicKeyImporterBIP32Path' is defined but never used  no-unused-vars

✖ 1 problem (1 error, 0 warnings)

@hwinsby
Copy link
Contributor Author

hwinsby commented Oct 30, 2023

Oh sorry about that! I think I can fix it quickly...

@hwinsby
Copy link
Contributor Author

hwinsby commented Oct 30, 2023

Ok @bucko13 I think it should be good to go now.

@bucko13
Copy link
Contributor

bucko13 commented Oct 30, 2023

So, I ran through some manual tests on the current commit (see version and hash in the bottom right hand corner).
Screenshot 2023-10-30 at 4 54 26 PM

Most things I tested worked:

  • importing xpub from trezor, coldcard, and ledger
  • Signed a testnet tx w/ a trezor in the test suite
  • Exported psbt for coldcard

I ran into a bug though when trying to upload a partially signed PSBT for a signing flow in the wallet. The field is disabled:
Screenshot 2023-10-30 at 5 21 51 PM

Whereas on the live version of caravan this is what it looks like:
Screenshot 2023-10-30 at 5 25 47 PM

I think this should be a quick fix though as I'm seeing this error in the console:

Warning: Failed prop type: Invalid prop `disableClick` of type `function` supplied to `Dropzone2`, expected `boolean`.

(Unfortunately caravan isn't as well tested as it should be, either that or the imminent typescript support would've helped catch this I bet).

Once I've confirmed that's working I think this should be good to go!

@hwinsby
Copy link
Contributor Author

hwinsby commented Oct 30, 2023

Gotcha. I'll work on it this evening! Sorry about this 😵

@hwinsby
Copy link
Contributor Author

hwinsby commented Oct 31, 2023

Looks like there was a pre-existing issue with one of the default propTypes in ColdCardFileReader.jsx , but it wasn't an issue until I updated to functional and passed in the props manually. Basically hasError was defaulted to PropTypes.bool instead of just false. Hopefully this should solve it.
Here's a screenshot of the dropzone after the fix:
image

Commit 70d0cb1

@bucko13
Copy link
Contributor

bucko13 commented Oct 31, 2023

Awesome. Thanks!

@bucko13
Copy link
Contributor

bucko13 commented Oct 31, 2023

Confirmed!

Screenshot 2023-10-31 at 11 15 15 AM

bucko13
bucko13 approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@bucko13 bucko13 merged commit 6139b9b into unchained-capital:master Oct 31, 2023
@hwinsby
Copy link
Contributor Author

hwinsby commented Oct 31, 2023

😶‍🌫️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bucko13 bucko13 bucko13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hwinsby @bucko13